Skip to content

[dotnet] Adding flag to enable SafariDriver logging. #16196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Aug 18, 2025
Merged

[dotnet] Adding flag to enable SafariDriver logging. #16196

merged 3 commits into from
Aug 18, 2025

Conversation

@diemol
Copy link
Member

@diemol diemol commented Aug 17, 2025 •
edited by qodo-merge-pro bot
Loading

User description

🔗 Related Issues

This is related to #12273. I mistakenly thought it was not possible to enable logging in SafariDriver.

💥 What does this PR do?

This option enables logging in SafariDriver, which only goes to a file in a hardcoded directory (~/Library/Logs/com.apple.WebDriver/).

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement

Description

  • Add Diagnose property to enable SafariDriver logging

  • Append --diagnose flag to command line arguments

  • Remove unused imports for cleaner code

Diagram Walkthrough

flowchart LR
  A["SafariDriverService"] --> B["Diagnose Property"]
  B --> C["--diagnose Flag"]
  C --> D["~/Library/Logs/com.apple.WebDriver/"]
Loading

File Walkthrough
Relevant files
Enhancement
SafariDriverService.cs
Add diagnostic logging support and cleanup imports             

dotnet/src/webdriver/Safari/SafariDriverService.cs

  • Add Diagnose boolean property with documentation
  • Modify CommandLineArguments to append --diagnose flag when enabled
  • Remove unused System.Net and System.Threading.Tasks imports
 +13/-3   

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Aug 17, 2025
@qodo-merge-pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

API Consistency

Consider whether the new nullable bool Diagnose should be non-nullable with a default (e.g., false) for a clearer public API and to match similar flags elsewhere in the codebase.

 
/// <summary>
/// Value to enable diagnose logging.
/// When set to true, the SafariDriver will be started with the --diagnose flag.
/// Logs will be written to ~/Library/Logs/com.apple.WebDriver/
/// </summary>
public bool? Diagnose  { get; set; }

/// <summary>
Argument Formatting

Ensure no unintended leading/trailing spaces or duplicate flags occur when appending " --diagnose"; verify base.CommandLineArguments handling and potential repeated appends across calls.

 
protected override string CommandLineArguments
{
    get
    {
        StringBuilder argsBuilder = new StringBuilder(base.CommandLineArguments);

        if (this.Diagnose is true)
        {
            argsBuilder.Append(" --diagnose");
        }

        return argsBuilder.ToString();
    }

@diemol diemol added this to the Selenium 4.36 milestone Aug 17, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 17, 2025 •
edited
Loading

PR Code Suggestions ✨

nvborisenko
nvborisenko reviewed Aug 17, 2025
View reviewed changes
nvborisenko
nvborisenko reviewed Aug 17, 2025
View reviewed changes
@diemol diemol merged commit 1283dd2 into trunk Aug 18, 2025
10 checks passed
@diemol diemol deleted the dotnet_enable_logging_safaridriver branch August 18, 2025 08:05
This was referenced Oct 2, 2025
Closed
Closed
Closed
Closed
Closed
Merged
Merged
Closed
Merged
Closed
Closed
Closed
Merged
Merged
Closed
Merged
Merged
This was referenced Oct 3, 2025
Closed
Closed
Closed
Merged
Closed
This was referenced Oct 20, 2025
Open
Merged
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nvborisenko nvborisenko nvborisenko approved these changes

Assignees

No one assigned

Labels

C-dotnet .NET Bindings Review effort 2/5

Projects

None yet

Milestone

Selenium 4.36

Development

Successfully merging this pull request may close these issues.

3 participants

@diemol @nvborisenko @selenium-ci